Skip to content

Distinguish delim kind to decide whether to emit unexpected closing delimiter #138554

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xizheyin
Copy link
Contributor

@xizheyin xizheyin commented Mar 16, 2025

Fixes #138401

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2025

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2025
@compiler-errors
Copy link
Member

r? compiler

@rustbot rustbot assigned estebank and unassigned compiler-errors Mar 21, 2025
@chenyukang
Copy link
Member

I changed this part last year, will review this PR later.

r? @chenyukang

@rustbot rustbot assigned chenyukang and unassigned estebank Apr 7, 2025
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2025
@chenyukang
Copy link
Member

I think this solution introduces some kind of regression on other scenarios(such as the cases I commented).

Maybe we should consider this as a special corner case and provide a special diagnostic for it, it is the pattern that one delimiter is missing the openning delimiter(it's in the inner part), except for this everything is ok, so we should suggest something like "missing openning delimiter ....".

such as:

{ ... {. ) ... }...} missing a inner (
{ ... [ ], ] ...} missing a inner [

@chenyukang
Copy link
Member

@rustbot author

@rustbot
Copy link
Collaborator

rustbot commented Apr 11, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@xizheyin
Copy link
Contributor Author

Thanks, I will revise it soon!

@xizheyin xizheyin changed the title [Lexer] Remove spurious unexpected delimiter error by matching remain… Distinguish delim kind to decide whether to emit unexpected closing delimiter Apr 12, 2025
@xizheyin
Copy link
Contributor Author

This seems like quite a lot of changes, I'll comment in the code to make it easier to REVIEW.

In the first commit, I added the test.

In the second commit I did some clean. I changed a variable name, the original name was inaccurate, I used open_delimiters instead of open_braces for identifying blocks, one should compare spans, not just brace'{'.

In the third commit, I make a distinction between the different delimiters before triggering a unexpected closing delimiter error. But I found that missing open delim note is what triggers in unexpected closing delimiter error.
Therefore, to emit missing open delim note, I also moved missing open delim note to
mismatched closing delimiter error, which can be emitted without delaying to
unexpected closing delimiter error.
And I moved function make_unclosed_delims_error
to src/lexer/diagnostics.rs for clean. And some other clean.

@xizheyin xizheyin requested a review from chenyukang April 12, 2025 11:02
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 12, 2025
@xizheyin
Copy link
Contributor Author

I've combined them into one line so it's more aesthetically pleasing.

mismatched closing delimiter, may missing open `(`

Since the mismatched closing delimiter note and the unclosed delimiter note are related, it may be the better to keep it.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 12, 2025
Copy link
Contributor Author

@xizheyin xizheyin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This time, I reserve the mismatched error when the situation is like {[)}.
And I remove the mismatched error only when close_delim_err, which means double close delims.

Meanwhile, I remove duplicate missing open ... for this delimiter in this file contains an unclosed delimiter error because mismatched errors have already emitted it.

@xizheyin
Copy link
Contributor Author

For this comment #138554 (comment)

Unfortually, <...> is not recognized as delimiters1, So the pattern here should be ()), the current state is reasonable.

As to why the <... > is set to delimiter, I think it's due to syntax like a>b and thus not parsed here. For example, encountering places where generic arguments should exist, and then recursively descending to parse <... >.

@chenyukang
Copy link
Member

For this comment #138554 (comment)

Unfortually, <...> is not recognized as delimiters1, So the pattern here should be ()), the current state is reasonable.

As to why the <... > is set to delimiter, I think it's due to syntax like a>b and thus not parsed here. For example, encountering places where generic arguments should exist, and then recursively descending to parse <... >.

got it, thanks for the explanation.

@chenyukang
Copy link
Member

I don't have other questions except for #138554 (comment)

@chenyukang
Copy link
Member

seems the latest commit #138554 add more noise.

0682e9e is good enough.

@xizheyin
Copy link
Contributor Author

This commit 0682e9e is for #138554 (comment). I use the nearest open delimiter.

The last commit a1d2557 is for comment #138554 (comment). In all the cases, we should emit missing open delimiter hints. Because,

  1. In mismatched error exists, the hint is not concrete enough.
  2. In mismatched error removed, the hint is then naturally needed.

Now, all the changes is

  1. Add a nearest open delimeter label in missing open delimeter hints
  2. remove false mismatched error in the cases like {}}

That's all we need.

But the code needs some clean. If everything is Ok, I will clean the code and squash commits. 😃

@xizheyin
Copy link
Contributor Author

seems the latest commit #138554 add more noise.

The last commit is necessary if one is in a conservative consideration, as #138401 seems to be just to address cases like, {}}. The exact explanation is in my last comment #138554 (comment).

@chenyukang
Copy link
Member

#138554 (comment)

I think we can remove those hint, since we reported mismatch above.

@xizheyin
Copy link
Contributor Author

Ok, I'll revise it later.

@xizheyin
Copy link
Contributor Author

But I have a concern that we only reported what went wrong in the mismatched error, but we didn't mention how to correct it. I think this hint complements this.

If you think it should be removed, I will remove the last commit later.

@chenyukang
Copy link
Member

But I have a concern that we only reported what went wrong in the mismatched error, but we didn't mention how to correct it. I think this hint complements this.

If you think it should be removed, I will remove the last commit later.

for a mismatched scenario, the hint is not complementing it:

{ .... ] may be fixed as { ..... } or [ .... ].

perfer to remove it.

@xizheyin
Copy link
Contributor Author

xizheyin commented Jul 18, 2025

Make sense! I removed it.

And I cleaned the code, and squashed them to 2 commits. Now, it should be what we want!

@chenyukang
Copy link
Member

Thanks!

@bors r=chenyukang

@bors
Copy link
Collaborator

bors commented Jul 18, 2025

📌 Commit 181c1bd has been approved by chenyukang

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2025
@xizheyin
Copy link
Contributor Author

Thank you for your patient guidance! :)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 18, 2025
Distinguish delim kind to decide whether to emit unexpected closing delimiter

Fixes rust-lang#138401
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 18, 2025
Distinguish delim kind to decide whether to emit unexpected closing delimiter

Fixes rust-lang#138401
bors added a commit that referenced this pull request Jul 18, 2025
Rollup of 9 pull requests

Successful merges:

 - #138554 (Distinguish delim kind to decide whether to emit unexpected closing delimiter)
 - #142673 (Show the offset, length and memory of uninit read errors)
 - #142693 (More robustly deal with relaxed bounds and improve their diagnostics)
 - #143382 (stabilize `const_slice_reverse`)
 - #143928 (opt-dist: make llvm builds optional)
 - #143961 (Correct which exploit mitigations are enabled by default)
 - #144050 (Fix encoding of link_section and no_mangle cross crate)
 - #144059 (Refactor `CrateLoader` into the `CStore`)
 - #144123 (Generalize `unsize` and `unsize_into` destinations)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handling of missing closing or opening delimiters, well distinct by type of delimiter
8 participants